DEPR: is_copy arg of take #30615

ryankarlos · 2020-01-02T04:28:17Z

closes DEPR: .take(..., is_copy=True); rename is_copy -> copy #27357
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

ryankarlos · 2020-01-02T05:45:02Z

Not sure why my changes are making these other tests break ?
Failed: DID NOT RAISE <class 'pandas.core.common.SettingWithCopyError'>

pandas/core/generic.py

jreback · 2020-01-03T01:10:43Z

looks good, needs a rebase; ping when green.

pep8speaks · 2020-01-03T01:19:37Z

Hello @ryankarlos! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-01-06 02:08:13 UTC

ryankarlos · 2020-01-03T04:08:40Z

@jreback do you know why the depr warning is making the test fail specifically for Linuxpy37 ?

pandas/tests/frame/methods/test_asof.py:27: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
pandas/core/generic.py:7009: in asof
    data = self.take(locs, is_copy=False)

and if i set this to data = self.take(locs) , it raises a SettingWithCopyError for all the builds due to this write on copy data.loc[missing] = np.nan

        if value == "raise":
>           raise com.SettingWithCopyError(t)
E           pandas.core.common.SettingWithCopyError: 
E           A value is trying to be set on a copy of a slice from a DataFrame.
E           Try using .loc[row_indexer,col_indexer] = value instead
E           
E           See the caveats in the documentation: http://pandas.pydata.org/pandas-docs/stable/user_guide/indexing.html#returning-a-view-versus-a-copy

pandas/core/generic.py:3684: SettingWithCopyError

TomAugspurger · 2020-01-03T13:43:18Z

The numpydev py37 build is the only one where we elevate warnings to errors which fail the build.

Most likely you'll need to look at that test and what is_copy is doing. If the test looks correct, you made need to replicate some of the is_copy=False behavior in the places that previously passed is_copy=False.

ryankarlos · 2020-01-04T12:23:04Z

thanks. @TomAugspurger @jreback found a workaround - all green now - let me know if any more changes needed.

jreback

unrelated routines, df.asof should not be showing a warning

jreback · 2020-01-04T18:44:16Z

pandas/tests/frame/methods/test_asof.py

+        with warnings.catch_warnings():
+            warnings.simplefilter("ignore", FutureWarning)
+
+            result = df.asof(dates)


why is this showing a warning? you need to fix internally so this is not the case

because the self.take(locs, is_copy=False) in def asof will now generate a warning after the deprecation change.
I could change it to default None (which sets is_copy=True) - but then that generates a SettingWithCopyError due to data.loc[missing] = np.nan in the block below

missing = locs == -1 data = self.take(locs) data.index = where data.loc[missing] = np.nan

I suppose I could temporarily suppress it

with pd.option_context('mode.chained_assignment', None): data.loc[missing] = np.nan

or create a deep copy after the take operation but not sure if thats the best approach ?

you simply step theu the code and out a copy where needed

jreback · 2020-01-04T18:44:33Z

pandas/tests/frame/methods/test_asof.py

-        expected = df.asof(dates)
-        tm.assert_frame_equal(result, expected)
+        with warnings.catch_warnings():
+            warnings.simplefilter("ignore", FutureWarning)


jreback · 2020-01-06T13:21:36Z

thanks @ryankarlos

jorisvandenbossche · 2020-01-07T10:35:41Z

This might have caused a regression in asof: https://pandas.pydata.org/speed/pandas/#timeseries.AsOf.time_asof?p-constructor='DataFrame'&commits=2a3d840d-c99dfea3 (note there are a bunch of other commits in the time span of the regression according to asv, but this is the most obvious one that touched asof)

jorisvandenbossche · 2020-01-07T10:36:47Z

pandas/core/generic.py

@@ -7011,7 +7023,8 @@ def asof(self, where, subset=None):

        # mask the missing
        missing = locs == -1
-        data = self.take(locs, is_copy=False)
+        d = self.take(locs)
+        data = d.copy()


Were there test failures if you didn't do this d.copy() step?

Yes, it was breaking builds with SettingwithCopyError see #30615 (comment) and lower bit of #30615 (comment)

jorisvandenbossche · 2020-01-07T15:33:26Z

Yes, it was breaking builds with SettingwithCopyError #30615 (comment)

I think this points to a problem in the current deprecation.

IMO, the goal of removing is_copy is to make this keyword unnecessary, in the idea that take should always return a copy. So that means you should never need to manually copy after doing a take.

WIth the current deprecation however, the user seems to loose this ability of not needing to take a copy. So I think, when deprecating/removing the is_copy arg, it should rather be the behaviour of is_copy=False that we should keep as the default.

jreback · 2020-01-08T01:00:28Z

the purpose of is_copy was to force a copy
take never had the always copy behavior of numpy - but it was mostly an internal method anyhow (eg you can use iloc)

so prob ok just to leave as is (maybe add a doc string)

jorisvandenbossche · 2020-01-08T09:54:00Z

the purpose of is_copy was to force a copy

I don't think this is correct, as is_copy=True/False did not influence whether a copy was made or not. What is_copy=True did, is setting self._is_copy to indicate to pandas it is referencing another object. So actually is_copy=True made it "less" a copy than is_copy=False.

With is_copy=False, you could indicate to pandas "this is really a copy already, don't give me annoying SettingWithCopyWarnings" without the need to do another explicit copy.

(the name of is_copy and the explanation in the docstring is just very confusing / wrong)

This reverts commit 7796be6.

ryankarlos changed the title ~~deprecate is_copy argument of take and remove is_copy=False usage~~ DEPR: dis_copy argument of take Jan 2, 2020

ryankarlos changed the title ~~DEPR: dis_copy argument of take~~ DEPR: is_copy arg of take Jan 2, 2020

ryankarlos force-pushed the DEPR/is_copy_arg_of_take branch from 6f4c0fc to 4368cd3 Compare January 2, 2020 05:06

charlesdong1991 reviewed Jan 2, 2020

View reviewed changes

pandas/core/generic.py Show resolved Hide resolved

jreback added the Deprecate Functionality to remove in pandas label Jan 3, 2020

jorisvandenbossche mentioned this pull request Jan 3, 2020

DEPR: log of deprecations in 1.x (to be removed in 2.0) #30228

Closed

ryankarlos force-pushed the DEPR/is_copy_arg_of_take branch from 7d503f5 to 101786a Compare January 3, 2020 01:19

ryankarlos force-pushed the DEPR/is_copy_arg_of_take branch 2 times, most recently from c9d5f26 to 4ed4028 Compare January 3, 2020 01:44

jreback added this to the 1.0 milestone Jan 3, 2020

ryankarlos force-pushed the DEPR/is_copy_arg_of_take branch from aea421f to 2fd97da Compare January 3, 2020 03:17

ryankarlos force-pushed the DEPR/is_copy_arg_of_take branch from 2fd97da to dcb8fd2 Compare January 4, 2020 05:17

jreback requested changes Jan 4, 2020

View reviewed changes

ryankarlos added 11 commits January 5, 2020 01:11

deprecate is_copy argument of take and remove is_copy=False usage

1e1b4f1

add test for depr is_copy

17ca686

add whats new note

9bd696c

Fix condition for is_copy to avoid breaking other tests

dabe051

linting

8142050

correct typing for is_copy arg

277d925

Fix CI errors

d8db903

catch warnings in test_asof

88dfc50

fix merge conflict and linting

7c533a1

revert warnings filtering in tests

87b1ba5

switch to is_copy default None in asof

6c1d495

ryankarlos force-pushed the DEPR/is_copy_arg_of_take branch from dcb8fd2 to 6c1d495 Compare January 5, 2020 03:15

ryankarlos added 2 commits January 6, 2020 01:50

fix merge conflict

0798283

remove blank line

e914148

jreback approved these changes Jan 6, 2020

View reviewed changes

jreback merged commit 7796be6 into pandas-dev:master Jan 6, 2020

jorisvandenbossche reviewed Jan 7, 2020

View reviewed changes

jorisvandenbossche mentioned this pull request Jan 7, 2020

DEPR: .take(..., is_copy=True); rename is_copy -> copy #27357

Closed

ryankarlos deleted the DEPR/is_copy_arg_of_take branch January 8, 2020 03:24

jorisvandenbossche mentioned this pull request Jan 10, 2020

PERF: performance regression in 1.0 compared to 0.25 #30790

Closed

4 tasks

jorisvandenbossche added a commit to jorisvandenbossche/pandas that referenced this pull request Jan 15, 2020

Revert "DEPR: is_copy arg of take (pandas-dev#30615)"

c854a8a

This reverts commit 7796be6.

jorisvandenbossche mentioned this pull request Jan 15, 2020

Revert "DEPR: is_copy arg of take (#30615)" #31032

Closed

jorisvandenbossche mentioned this pull request Jan 23, 2020

API: DataFrame.take always returns a copy #30784

Merged

lukemanley mentioned this pull request Oct 21, 2022

DEPR: is_copy arg from take #49221

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEPR: is_copy arg of take #30615

DEPR: is_copy arg of take #30615

ryankarlos commented Jan 2, 2020 •

edited

Loading

ryankarlos commented Jan 2, 2020

jreback commented Jan 3, 2020

pep8speaks commented Jan 3, 2020 •

edited

Loading

ryankarlos commented Jan 3, 2020 •

edited

Loading

TomAugspurger commented Jan 3, 2020

ryankarlos commented Jan 4, 2020 •

edited

Loading

jreback left a comment

jreback Jan 4, 2020

ryankarlos Jan 5, 2020 •

edited

Loading

jreback Jan 5, 2020

jreback Jan 4, 2020

jreback commented Jan 6, 2020

jorisvandenbossche commented Jan 7, 2020

jorisvandenbossche Jan 7, 2020

ryankarlos Jan 7, 2020 •

edited

Loading

jorisvandenbossche commented Jan 7, 2020

jreback commented Jan 8, 2020

jorisvandenbossche commented Jan 8, 2020

DEPR: is_copy arg of take #30615

DEPR: is_copy arg of take #30615

Conversation

ryankarlos commented Jan 2, 2020 • edited Loading

ryankarlos commented Jan 2, 2020

jreback commented Jan 3, 2020

pep8speaks commented Jan 3, 2020 • edited Loading

Comment last updated at 2020-01-06 02:08:13 UTC

ryankarlos commented Jan 3, 2020 • edited Loading

TomAugspurger commented Jan 3, 2020

ryankarlos commented Jan 4, 2020 • edited Loading

jreback left a comment

Choose a reason for hiding this comment

jreback Jan 4, 2020

Choose a reason for hiding this comment

ryankarlos Jan 5, 2020 • edited Loading

Choose a reason for hiding this comment

jreback Jan 5, 2020

Choose a reason for hiding this comment

jreback Jan 4, 2020

Choose a reason for hiding this comment

jreback commented Jan 6, 2020

jorisvandenbossche commented Jan 7, 2020

jorisvandenbossche Jan 7, 2020

Choose a reason for hiding this comment

ryankarlos Jan 7, 2020 • edited Loading

Choose a reason for hiding this comment

jorisvandenbossche commented Jan 7, 2020

jreback commented Jan 8, 2020

jorisvandenbossche commented Jan 8, 2020

ryankarlos commented Jan 2, 2020 •

edited

Loading

pep8speaks commented Jan 3, 2020 •

edited

Loading

ryankarlos commented Jan 3, 2020 •

edited

Loading

ryankarlos commented Jan 4, 2020 •

edited

Loading

ryankarlos Jan 5, 2020 •

edited

Loading

ryankarlos Jan 7, 2020 •

edited

Loading